-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a shortcode for nocookie youtube embeds #651
Conversation
|
||
return sprintf( | ||
// `allow` settings copied from youtube-provided embed code. | ||
'<iframe style="aspect-ratio: 16/9;" src="%s" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" referrerpolicy="strict-origin-when-cross-origin" allowfullscreen %s></iframe>', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor. Should this use numbered placeholders?
* | ||
* @param array $attr Shortcode attributes array, can be empty if the original arguments string cannot be parsed. | ||
* @param string $content Content inside shortcode tags. | ||
* @param string $tag Shortcode name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core uses the parameter name $shortcode
for this. Should we also follow that standard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't see that parameter being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core uses $tag
elsewhere, so I don't think it's really a standard. This parameter is only useful if we're using the same callback for different shortcodes, which doesn't apply here. I'll remove it.
|
||
return sprintf( | ||
// `allow` settings copied from youtube-provided embed code. | ||
'<iframe style="aspect-ratio: 16/9;" src="%s" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" referrerpolicy="strict-origin-when-cross-origin" allowfullscreen %s></iframe>', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would enforcing 16/9
but allowing users to set width/height be an issue for editors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to merge this so I can build out the WordCamp pages, but we can iterate if needed. |
See WordPress/wporg-main-2022#493 (comment) — We'd like to use the "privacy-enhanced" youtube embeds, but the domain used is not supported by the core embeds. I've put this code in wporg-mu-plugins because I'll also want to use it on WCUS's livestream pages; and I can see needing it on other pages.
Hopefully the core ticket will be fixed at some point, and we can phase this out.
To test
src
URL[youtube-nocookie]COPIED URL[/youtube-nocookie]
Try other URLs, it should only create the iframe for youtube-nocookie.com domains.